Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PMREMGenerator: Set minFilter to NearestFilter. #23468

Merged
merged 1 commit into from
Feb 14, 2022
Merged

PMREMGenerator: Set minFilter to NearestFilter. #23468

merged 1 commit into from
Feb 14, 2022

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Feb 11, 2022

Related issue: google/model-viewer#3145

Description

@kenrussell suggested this workaround for cases where users override the texture.anisotropy setting from the Nvidia settings panel which produce seams when generating the atlas.

@elalish Can you think of any side effects this PR may have?

@mrdoob mrdoob added this to the r138 milestone Feb 11, 2022
@elalish
Copy link
Contributor

elalish commented Feb 14, 2022

Huh, good question. I don't have a clear idea what would trigger the max vs min filter in this case; the pixel derivatives will be pretty crazy. Best bet is to look at a nice mirror ball (zero roughness) before and after and see if the reflection looks worse. If you can't tell a difference, then I guess it's fine.

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 14, 2022

@mrdoob mrdoob merged commit 6386db3 into dev Feb 14, 2022
@mrdoob mrdoob deleted the pmrem branch February 14, 2022 18:31
@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 17, 2022

This PR noticeably changed the visual result of webgl_pmrem_test:

https://threejs.org/examples/webgl_pmrem_test
https://raw.githack.com/mrdoob/three.js/dev/examples/webgl_pmrem_test.html

Is the new output considered as correct?

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 17, 2022

@elalish
Copy link
Contributor

elalish commented Feb 17, 2022

@Mugen87 Both of those examples are positively identical on my macbook. Can you share screenshots and what GPU you're testing on?

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 17, 2022

image

Prod:
image
Dev:
image

Prod:
image
Dev:
image

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 17, 2022

I also see a difference with a Pixel 4a and latest Chrome.

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 17, 2022

I can reproduce 👍
The renders are a bit darker now. Working on a fix.

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 17, 2022

It's also worth noting that #23428 changed the cubeuv configuration so the renders will change sligthly from 137 to 138:

Before After
Screen Shot 2022-02-17 at 12 36 55 PM Screen Shot 2022-02-17 at 12 36 37 PM

@elalish
Copy link
Contributor

elalish commented Feb 17, 2022

As for the change; the pixel values themselves should all be identical, merely arranged differently. So any difference should be scrutinized. I'm still having trouble seeing the differences in those renders; can someone circle something?

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 17, 2022

I guess @Mugen87 and I have super eyes at this point 😁

image

image

https://www.diffchecker.com/image-diff/

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 17, 2022

As for the change; the pixel values themselves should all be identical, merely arranged differently.

There are less pixels now. The tiles are squashed vertically.
Which means that the resulting accumulation is unlikely to be 100% the same.

@elalish
Copy link
Contributor

elalish commented Feb 17, 2022

No, they only look squashed. The new texture is larger than the old. And not square.

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 17, 2022

Ahh, I see. I'll have to update the debug code with the new size then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants